Skip to content

fix(ui): stop the default-model picker spinning on every switch - #3828

Open
liuxiaocs7 wants to merge 1 commit into
apache:mainfrom
liuxiaocs7:fix/default-model-picker-spinner
Open

fix(ui): stop the default-model picker spinning on every switch#3828
liuxiaocs7 wants to merge 1 commit into
apache:mainfrom
liuxiaocs7:fix/default-model-picker-spinner

Conversation

@liuxiaocs7

@liuxiaocs7 liuxiaocs7 commented Aug 25, 2026

Copy link
Copy Markdown
Member

Summary

Switching the default model in Settings › 通用 › 任务默认 › 默认模型 spun a
loading spinner on the picker trigger for the whole save.

ModelPicker drove the Astryx Selector through its async changeAction
prop. On that path the Selector holds the trigger aria-busy (a <Spinner>)
until its built-in optimistic value catches up to the controlled value — i.e.
the whole setDefaultModel + connection-refresh round-trip. The sibling
permission-mode and thinking-level selectors never spun because they use the
synchronous onChange path.

This switches ModelPicker to onChange (fire-and-forget, never busy). On that
path Astryx no longer advances its own optimistic value, so the Settings row
supplies the "reflect the pick immediately" half of #3827 itself — otherwise
the trigger would sit on the old model (disabled) until the refresh landed.

Reflecting the pick without a stale/premature race

The row keeps a local optimistic value (useOptimisticSelection) shown the
instant a model is picked. It is cleared by a read barrier keyed on the
connections read generation
— not a value compare and not a snapshot
reference (both are ambiguous: a snapshot ref only proves a read finished, so
a read already in flight at pick time, returning the pre-write value, would
clear the pick as soon as it commits).

  • begin(next) shows the pick; the barrier is disarmed.
  • settle(floor) arms the barrier at the reads issued once the write is durable.
  • Only a read issued strictly after the write (the row's own refresh) clears
    it; a read issued at/before the write (generation ≤ floor) never does.

Resulting behavior, all correct:

case shown
our save accepted authoritative value
in-flight pre-write read commits the old value pick kept (barrier disarmed / gen ≤ floor)
concurrent external write, or A→B→A restore authority (the accepted post-write read)
refresh lands no accepted read (failure/invalidation) pick kept — the write persisted it
setDefaultModel threw rolled back to authoritative

The committed connections read generation is threaded from the settings request
authority (settings-request-authority.tssettings-surface.tsx) to the row.
The now-unused loading prop is dropped from ModelPicker.

Verification

Ran locally (macOS, Node v24) against the current main:

  • @maka/ui unit test use-optimistic-selection.test.tsx — 8 cases: instant
    show; in-flight read before settle; in-flight read at/under the floor;
    post-write refresh clears to the pick; concurrent external write; A→B→A;
    refresh-lands-nothing keeps the pick; cancel on a thrown write.
  • @maka/ui + @maka/desktop typecheck (incl. tsconfig.storybook.json) — clean.
  • biome lint — clean.
  • Storybook render smoke (build-storybook + smoke:storybook) — passed (195 stories).

On a no-spinner unit test: the no-spin behavior is now structural —
ModelPicker has no changeAction/loading code path to spin. A faithful
regression test needs a real browser: Astryx's spinner comes from
startTransition + useOptimistic, which do not surface as aria-busy under
node:test+linkedom (verified — a probe still passed after flipping to
changeAction, so it would have been a false guard and was not kept). The
optimistic-state logic is unit-tested above; the wiring is guarded structurally.

Did not run the full desktop Playwright e2e locally (renderer change); CI covers it.

AI use

  • Generative tooling made a substantive contribution

Tool(s): Claude Code — read the installed Astryx Selector, designed the
read-generation barrier, implemented it, and authored the unit test. The human
contributor of record (@liuxiaocs7) reviewed the work and owns its accuracy and
licensing. A Generated-by: Claude Code trailer is on the commit; please retain
it on the squash commit.

Checklist

  • Tests cover the change and fail without it (optimistic-state logic)
  • Lint, typecheck and the affected suites pass locally
  • After screenshot/recording — to be attached by @liuxiaocs7 (cannot capture Electron in the review environment)

Does this PR entail a change in behavior?

  • Yes — described under Summary above

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update on d5cb4e8b1b:

[P2] Optimistic pendingDefaultModel never clears on external Host change

pendingDefaultModel only clears when === selectedValue. If another window writes C after this window picked B, refresh updates selectedValue to C (B!=C) so pending B stays and keeps covering ModelPicker — Host authority is C but UI shows B or "unset" (stale/authority split).

Fix: bind optimistic to Host revision/target and discard on non-matching accepted refresh.

Checks on d5cb4e8b1bad49ae701f09fdcedc7b496ed65df5 are test: SUCCESS — code is NO-GO.

简体中文外部写入后乐观值不清理导致显示不一致。

liuxiaocs7 added a commit to liuxiaocs7/maka that referenced this pull request Aug 25, 2026
pendingDefaultModel only cleared when it equaled selectedValue, so a
concurrent external write (another window setting a different default)
that the Host accepted stranded the picker on the stale pick forever:
Host authority was C but the trigger kept showing B (authority split).

Drop the optimistic overlay as soon as the save's refresh lands, so the
trigger always settles on the authoritative snapshot whether it accepted
this pick or an external one. Preserves the no-spin onChange path and the
instant-label UX; the failure-path clear is unchanged.

Addresses review feedback on apache#3828.

Generated-by: Claude Code
@liuxiaocs7

Copy link
Copy Markdown
Member Author

Thanks — confirmed and fixed in c9c8cb3.

Root cause matches your read. props.connections/defaultSlug don't only
update via this row's own onRefresh()settings-surface.tsx keeps a live
connectionsBridge.subscribeEvents(() => reloadConnections(...)) subscription,
and reloadConnections is gated by runtimeHostRequestAuthority so the
last-accepted Host snapshot wins. So an external write of C can move
selectedValue to C independently of this window. The old clear ran only on
pendingDefaultModel === selectedValue, so when the accepted value diverged
from the pick (B != C) the optimistic overlay was stranded forever and kept
covering the trigger — authority C, UI B. (Same strand also applied if the Host
normalized/rejected the pick to a different value.)

Fix: drop the optimistic overlay as soon as the save's refresh lands, rather
than only when it happens to equal the pick. After await onRefresh() the Host
snapshot is authoritative regardless of which write it accepted, so the
trigger now settles on it — this pick or an external one — and can never pin a
stale value. The no-spin onChange path and the instant-label UX are unchanged,
and the failure-path clear is untouched.

-  useEffect(() => {
-    if (pendingDefaultModel !== null && pendingDefaultModel === selectedValue) {
-      setPendingDefaultModel(null);
-    }
-  }, [pendingDefaultModel, selectedValue]);
   ...
       await props.onRefresh();
+      // Host snapshot is now authoritative — whether it accepted this pick or a
+      // concurrent external write. Drop the overlay so the trigger settles on it.
+      if (mountedRef.current) setPendingDefaultModel(null);

Not adding a dedicated regression test: this optimistic logic lives in the
desktop renderer (general-settings-page.tsx), which has no renderer unit
harness — apps/desktop's test only runs the main process, and the existing
guard is a packages/ui story for the isolated ModelPicker no-spin behavior,
not this page-level state. A faithful guard would need either a page-level
Playwright e2e or extracting the clear into a testable hook; happy to do the
extraction in a follow-up if you'd prefer it gated by a unit test.

Verified locally: renderer + storybook typecheck, @maka/ui typecheck, and
biome lint all clean.

简体中文已确认并修复(c9c8cb3)。根因如你所述:连接快照会经 subscribeEvents → reloadConnections 被外部写入独立刷新,旧逻辑仅在乐观值等于 selectedValue 时清除,导致外部值分歧时乐观遮盖永久粘滞(权威 C、界面 B)。 改为在 onRefresh() 落地后即撤下遮盖,让触发器始终收敛到权威快照,无论其接受的是 本次选择还是外部写入;不引入 spinner。未加专门回归测试:该逻辑位于无单测工装的桌面渲染层, 如需以单测把关,可在后续将清除逻辑抽为可测 hook。

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update on c9c8cb311b:

[P2] Unconditional clear of optimistic leaves stale snapshot on refresh failure

GeneralDefaultsCard clears pendingDefaultModel after await onRefresh() even when reloadConnections swallowed getSnapshot failure or was invalidated without accepted snapshot. Host now holds B but picker reverts to old A with stale isVerified, allowing decisions from wrong default.

Fix: only clear on successful accepted snapshot; handle failure/invalidation.

Checks on c9c8cb311be7ebc75f4d255b1ac7bac1b6d933d4 are test: FAILURE — not green.

简体中文刷新失败后仍清理导致回退。

liuxiaocs7 added a commit to liuxiaocs7/maka that referenced this pull request Aug 26, 2026
…he value

The previous clear ran unconditionally after onRefresh, so it reverted the
picker to the stale old value whenever reloadConnections swallowed a
getSnapshot failure or was invalidated without an accepted snapshot — Host
held the newly saved model but the trigger snapped back to the old one.

Clear the optimistic overlay only when an accepted snapshot moves the
server-derived value off the pre-pick baseline. The trigger then settles on
the authoritative value (this pick or a concurrent external write) and never
reverts to a stale value while a refresh is still unconfirmed. The no-spin
onChange path, the instant-label UX, and the failure-path clear are unchanged.

Addresses review feedback on apache#3828.

Generated-by: Claude Code
@liuxiaocs7

Copy link
Copy Markdown
Member Author

Good catch — valid, and fixed in a1b900f.

On the P2 (unconditional clear regresses on refresh failure). Confirmed:
onRefreshreloadConnections swallows a getSnapshot failure (its catch
sets a failed resource state, doesn't throw) and early-returns on
!acceptsConnectionsRead(ticket) / unmount without updating connections. So
await onRefresh() can resolve with selectedValue still on the old value,
and my unconditional clear then reverted the trigger to stale A even though the
save to B succeeded. My earlier === selectedValue guard handled that case
but missed the external-divergence case; the correct fix has to cover both.

Fix: clear the overlay only when an accepted snapshot moves
selectedValue off the pre-pick baseline, not merely when onRefresh()
resolves:

+  const pendingBaselineRef = useRef<string>("");
   ...
+  useEffect(() => {
+    if (pendingDefaultModel !== null && selectedValue !== pendingBaselineRef.current) {
+      setPendingDefaultModel(null);
+    }
+  }, [pendingDefaultModel, selectedValue]);
   ...
     setSaving(true);
+    pendingBaselineRef.current = selectedValue;
     setPendingDefaultModel(nextValue);

Resulting behavior:

case selectedValue after overlay shown
this save accepted (B) B (≠ baseline A) cleared B
external write accepted (C) C (≠ A) cleared C ✓ (original P2)
refresh failed / invalidated still A (= baseline) kept B ✓ (this P2 — Host holds B)
setDefaultModel threw A cleared in catch A

A later accepted snapshot (via subscribeEvents) moves the value off the
baseline and clears the overlay, so the failure case self-heals. No-spin
onChange path, instant-label UX, and the failure-path clear are unchanged.

On the red test check. The two failures are unrelated to this
settings-only change — both are flaky e2e on other surfaces:

  • composer-plus-menu-stability.spec.ts:262locator.click: Timeout waiting
    for the composer Plan menuitemcheckbox to become enabled (bridge-latch
    timing).
  • workhub-layout.spec.ts:22.workhub-result not visible within 10s
    (WorkHub submit→result timing). Its setDefaultModel calls are in a
    different, passing test and go through the bridge, not this picker UI.

Neither opens Settings or the ModelPicker, and the parent commit d5cb4e8b1
was test: SUCCESS. A settings-page React state change can't affect the
composer/WorkHub surfaces. The re-run on a1b900f should confirm. Locally:
renderer + storybook typecheck and biome lint clean.

简体中文已修复该 P2(a1b900f)。原无条件清除会在刷新失败/失效时 误回退到旧值(Host 已是 B、界面却退回 A),因为 reloadConnections 会吞掉 getSnapshot 失败并在失效时不更新 connections。改为:仅当被接受的快照把 selectedValue 移离选择前的基线时才清除遮盖——落在本次选择显示 B,落在外部写入显示 C, 刷新失败则保留 B(与 Host 一致),后续快照会自愈。CI 两个失败与本改动无关,是 composer / WorkHub 的既有 e2e flake(父提交 d5cb4e8 为 test: SUCCESS),重跑应恢复。

liuxiaocs7 added a commit to liuxiaocs7/maka that referenced this pull request Aug 26, 2026
…ectors

Reviewers flagged that inferring "my save's authoritative refresh has landed"
by comparing the model value is racy: an external restore to the pre-pick value
(ABA) never clears the overlay, and an unrelated accepted snapshot can clear it
early. Rather than track a refresh generation, drop the optimistic overlay
entirely — the default-model row now mirrors its sibling selectors (permission
mode, thinking level): value follows the authoritative connections snapshot,
disabled during save, no local optimism. That removes every stale/premature
state by construction; the trigger updates when the refresh lands, exactly like
the siblings.

Net change from base is now a single line (drop loading={saving}); the actual
spinner fix is the ModelPicker changeAction->onChange switch, which the
never-settling story guards.

Addresses review feedback on apache#3828.

Generated-by: Claude Code
@liuxiaocs7
liuxiaocs7 force-pushed the fix/default-model-picker-spinner branch 2 times, most recently from b252396 to d3ceb83 Compare August 26, 2026 08:10
@M4n5ter
M4n5ter force-pushed the fix/default-model-picker-spinner branch 3 times, most recently from 86152c5 to 8618fde Compare August 26, 2026 09:52
@liuxiaocs7
liuxiaocs7 force-pushed the fix/default-model-picker-spinner branch from 8618fde to e32bc4a Compare August 26, 2026 14:48

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What this PR does: switching the default model in Settings › 通用 › 任务默认 › 默认模型 spun a spinner on the picker trigger for the whole save. ModelPicker drove the Astryx Selector through its async changeAction prop, whose built-in optimistic value holds the trigger aria-busy until the controlled value catches up — i.e. for the entire setDefaultModel + connection-refresh round trip. This switches ModelPicker to the synchronous onChange path, matching the sibling permission-mode and thinking-level selectors, and drops loading={saving} at the call site. It adds a Storybook regression story that pins a never-resolving save and asserts the trigger is not left aria-busy.

The spinner half is correct and the story is a real guard. But I read the installed Selector implementation and the trade it makes is the opposite of what the PR body describes, so I do not think this closes #3827 yet.

P2 — this removes the spinner by making the trigger label lag, which is the other half of the reported bug

packages/ui/src/model-picker.tsx:100, apps/desktop/src/renderer/settings/general-settings-page.tsx:659

Reachability ①, every switch.

In node_modules/@astryxdesign/core/dist/Selector/Selector.js:

const commitValue = useCallback(newValue => {
  onChange?.(newValue);
  if (changeAction) {
    startTransition(async () => {
      setOptimisticValue(newValue);
      await changeAction(newValue);
    });
  }
}, [...]);

setOptimisticValue runs only on the changeAction branch. The trigger's label comes from valueContent, which renders selectedItem, which is selectableItems.findIndex(item => item.value === optimisticValue). And the Spinner is rendered as a sibling of valueContent, not in place of it.

So the actual before/after is:

spinner trigger label
before yes, for the whole save new model, immediately (optimistic)
after no old model until setDefaultModel + onRefresh() resolve

disabled={saving} is kept, so during that window the row shows the old model on a disabled trigger — on a slow Runtime Host round trip the pick reads as if it did not take.

#3827 asks for both: "selecting a model reflects the choice immediately with no spinner". This PR delivers the second and gives up the first.

The PR body says it "reflects the pick optimistically in the Settings row so the label updates instantly instead of waiting for the refresh" — that change is not in the diff. general-settings-page.tsx only loses the loading={saving} line; value={selectedValue} is still derived purely from props.defaultSlug / props.connections. Did an earlier revision carry it?

The fix is small and is exactly what the body already promises: a local pending value in GeneralDefaultsCard, preferred over selectedValue while saving, cleared when the refresh lands or the save fails.

Note the new story cannot catch this: it pins value fixed and asserts only not aria-busy, so it stays green whether or not the label ever updates.

Ungraded

ModelPicker has exactly one production consumer (this Settings row) — nothing else in apps or packages renders it. With loading={saving} gone, the loadingisLoading path now has no production caller, while the new comment says "spinning is opt-in via the explicit loading prop". Either keep it and say it is currently unused, or drop the prop.

AI use: Claude Code assisted with reading the installed Astryx Selector implementation; the verification and conclusions are my own.

简体中文

这个 PR 在做什么:Settings › 通用 › 任务默认 › 默认模型 切换模型时,触发器整个保存期间转圈。ModelPicker 用的是 Astryx Selector 的异步 changeAction,其内建乐观值会让触发器保持 aria-busy 直到受控 value 跟上,也就是整个 setDefaultModel + 连接刷新往返。这个 PR 改用同步 onChange(与相邻的权限模式、思考级别选择器一致),并在调用点去掉 loading={saving},另加一个 Storybook 回归 story。

去掉转圈这半是对的,story 也是真的守护。但我读了安装版 Selector 的实现,它做的取舍与 PR 描述相反,所以我认为还不能算关掉 #3827

P2setOptimisticValue 只在 changeAction 分支执行;触发器标签 valueContent 取自 selectedItem,而 selectedItemoptimisticValue 派生;SpinnervalueContent兄弟节点,不是替换它。所以实际是——改之前:转圈,但标签立刻变成新模型;改之后:不转圈,但标签要等保存和刷新落地才更新,且期间 disabled={saving} 让触发器处于禁用态,在 Host 往返慢时看起来像"这次选择没生效"。而 #3827 的 Expected 两者都要。

PR 描述里那句"reflects the pick optimistically in the Settings row so the label updates instantly" 在 diff 里并不存在:general-settings-page.tsx 只少了 loading={saving} 一行,value={selectedValue} 仍纯由 props 派生。是不是早期版本里有、后来掉了?

修法就是描述里已经承诺的那件事:在 GeneralDefaultsCard 里加一个本地 pending value,saving 期间优先显示它,刷新落地或保存失败时清掉。另外新 story 抓不到这个回归——它把 value 钉死,只断言 not aria-busy

不计分ModelPicker 全仓只有这一个生产消费者,删掉 loadingloading/isLoading 这条通路已无生产调用者,而新注释还写着 "spinning is opt-in via the explicit loading prop"。要么保留并注明当前未使用,要么把这个 prop 一起删掉。

@liuxiaocs7
liuxiaocs7 force-pushed the fix/default-model-picker-spinner branch from e32bc4a to 0d4175e Compare August 26, 2026 18:45
@github-actions github-actions Bot added the effort/M Under 500 readable lines label Aug 27, 2026
Settings > General > default model drove Astryx's Selector via the async
`changeAction` prop, which holds the trigger's built-in optimistic busy state
(a spinner) for the whole setDefaultModel + connection-refresh round trip.

Switch ModelPicker to the synchronous `onChange` path so the trigger never
spins. On that path Astryx no longer advances its own optimistic value, so the
Settings row supplies the "reflect the pick immediately" half of apache#3827 itself
via useOptimisticSelection: the pick shows the instant it is chosen and is
cleared by a read barrier keyed on the connections read GENERATION, not a value
or snapshot-reference compare. begin() shows the pick; settle() arms the barrier
at the reads issued once the write is durable; only a read issued strictly after
that (the caller's own refresh) clears it. So an in-flight pre-write read
returning the old value cannot clear the pick; a concurrent external write or
the prior value restored (A->B->A) resolves to authority; a refresh that lands
no accepted read keeps the pick (the write already persisted it); a thrown write
rolls back.

Thread the committed connections read generation from the settings request
authority down to the row. Drop the now-unused `loading` prop from ModelPicker.

Cover the optimistic states with a packages/ui unit test. The no-spinner wiring
is structural (ModelPicker has no changeAction/loading path) and can only be
exercised faithfully in a browser; node:test cannot drive Astryx's
transition/optimistic busy state, so no misleading unit assertion is added.

Fixes apache#3827

Generated-by: Claude Code
@liuxiaocs7
liuxiaocs7 force-pushed the fix/default-model-picker-spinner branch from 0d4175e to 5c0140c Compare August 27, 2026 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants